-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert Box Module to Use nanobind #1256
Conversation
for more information, see https://pre-commit.ci
If you're considering switching away from Cython you might want to consider nanobind instead. The benchmarks look quite good and the API is nearly identical. |
Thanks for sharing, I wasn't aware of nanobind. It looks perfect for what we do. ... and let the endless cycle of rewriting our codes continue ... |
The ndarray class looks to be great for interoperatibility between python and C++, which will be perfect for freud! |
for more information, see https://pre-commit.ci
I was only able to go through a small portion of PR. Will give it another go soon. |
Co-authored-by: Jen Bradley <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. It seems that, besides changes on the python side, we only have to change the data types to the nanobind array on c++ side + some minor tweaks.
One thing that I don't understand is why there are several sets of cpp functions such as makefraction and makefractionalPython. I can see that the "python" functions are used for exporting to python, but why is there a need for the non "python" function? Couldn't the logic be condensed into a single function? it seems that the main difference between the two functions is that non-python functions operate on a single set of data while the python functions do that for the whole system. Could we have either a single function for both use cases or if that is not possible, better names would also help (ie calling them _single vs. whole system or something like that?)
Generally speaking, there are 3 kinds of functions:
Type (1) exists to increase code readability and reduce code duplication (they are re-used sometimes in multiple places). Type (2) is largely leftover from cython-era freud, but could provide a C++ interface for the box class if at some point we decide to provide that. If we don't care about preserving something that could be easier to turn into a C++ API later, we could remove this type of function. Type (3) is so C++ and python can talk to each other with nanobind. When exporting functions, both pybind11 and nanobind cannot resolve two functions with the same name, so the suffix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I see no good reason to remove any of the 3 types of functions.
One solution is to implement the first 2 methods in The 3rd function in this case wouldn't be a member function. It could either be a static function that behaves like a member function (taking a |
Other than cluttering I haven't separated out the nanobind export modules (i.e. _box.cpython...) from the C++ library (libfreud.so) on this PR. I do this in the parallel module branch (which is also ready to go once this PR is merged), so it may be easier to wait until the next PR to make those changes. |
Certainly. If we decide to take this approach, this is a rearrange we could do after the next PR. Other than clutter, one possible downside I see is that Even if the parts of nanobind needed by It is really a question of how strongly do we want to support potential C++-only use-cases. If we think there is even a small chance users would like this in the future, it will be worth the small amount of extra effort during this rewrite. Refactoring the cluttered |
Ok, then let's do this on the next PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks like a clean port.
I have a number of recommendations to improve the code and a few points to discuss further.
Co-authored-by: Joshua A. Anderson <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tommy-waltmann, this looks great and is exactly what I have been suggesting! This pattern will make a really clean separation between the C++ library and the Python bindings. If we applied this pattern in HOOMD, for example, we would no longer need all those PYBIND11_EXPORT
macros.
for more information, see https://pre-commit.ci
Co-authored-by: Joshua A. Anderson <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This establishes a good pattern. We are ready to move on to the later modules.
Description
This PR changes the box module to use pybind11 instead of Cython. It also establishes a new pattern in the cmake code which other modules will be able to follow for when they are converted.
Motivation and Context
Cython is old and causing us a lot of developer headaches, so we are converting freud to use pybind11 instead.
How Has This Been Tested?
All old tests pass. Please build freud from source and test the box module manually for now.
Types of changes
Checklist: